Distributor trait #700
Distributor trait #700
Conversation
Introduce a distributor that knows how to partition containers across multiple pushers. This moves the partition logic from container builders into a bespoke trait and implementation instead of mixing two different concepts, building containers, and partitioning them. It allows us to implement a distributor in the future that only partitions by key, for example when the container doesn't have easy access by row. Removes the old `partition` function from container builders. Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Distributor trait that separates partitioning logic from container building responsibilities. This refactoring extracts the partition functionality previously embedded in container builders into a dedicated trait and implementation.
- Introduces
Distributortrait for partitioning containers across multiple pushers - Implements
DrainContainerDistributoras a hash-based distributor - Refactors
Exchangeto use the new distributor architecture instead of container builder partitioning
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| timely/src/dataflow/channels/pushers/exchange.rs | Adds Distributor trait and DrainContainerDistributor implementation, refactors Exchange to use distributor |
| timely/src/dataflow/channels/pact.rs | Updates ExchangeCore to use DrainContainerDistributor with new type signature |
| container/src/lib.rs | Removes partition method from ContainerBuilder trait |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Moritz Hoffmann <[email protected]>
frankmcsherry
left a comment
frankmcsherry
left a comment
There was a problem hiding this comment.
Looks good. Minor clarity comments. Holding of on any merging to sequence with the other active PR.
| if self.builders.len() != pushers.len() { | ||
| self.builders.resize_with(pushers.len(), Default::default); | ||
| } |
There was a problem hiding this comment.
Is this .. helpful or should this assert instead? I'm not sure how this gets used, and .. maybe this is a convenient way to initialize things I can't tell.
There was a problem hiding this comment.
Like, probably not going to happen .. but if this ever truncates then there may be a problem, if container builders get dropped holding on to records. Asserting seems better, or at least asserting if this would ever reduce the length.
There was a problem hiding this comment.
I added the number of peers as part of the constructor. Seems better than to resize at runtime. Also added a debug assert to make sure the lengths match.
Signed-off-by: Moritz Hoffmann <[email protected]>
Signed-off-by: Moritz Hoffmann <[email protected]>
2 participants
Introduce a distributor that knows how to partition containers across multiple pushers. This moves the partition logic from container builders into a bespoke trait and implementation instead of mixing two different concepts, building containers, and partitioning them.
It allows us to implement a distributor in the future that only partitions by key, for example when the container doesn't have easy access by row.
Removes the old
partitionfunction from container builders.